Conversation
…equence on number on LISTEN->SYN_RCVD transition
the same tuple already exists
operations (and, or etc...)
There was a problem hiding this comment.
Pull request overview
This PR addresses additional fuzzer-found issues in the TCP/IP stack, focusing on wrap-safe TCP sequence arithmetic and more robust TCP connection state handling.
Changes:
- Introduces a helper for TCP sequence subtraction and uses it in OOO/recv trimming paths.
- Adjusts option parsing arithmetic to avoid unsigned underflow and initializes
snd_unaon SYN-RCVD. - Adds SYN duplicate-rejection logic and new unit regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/wolfip.c | Implements wrap-aware TCP sequence handling, SYN duplicate rejection, and SYN-RCVD initialization fixes. |
| src/test/unit/unit.c | Adds regression tests validating snd_una initialization and duplicate SYN behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Subtract two TCP sequence numbers: a - b (wraps at 2^32) */ | ||
| static inline uint32_t tcp_seq_diff(uint32_t a, uint32_t b) | ||
| { | ||
| if (a >= b) | ||
| return a - b; | ||
| return UINT32_MAX - (b - a - 1); |
There was a problem hiding this comment.
tcp_seq_diff() returns uint32_t, but at least one call site treats the result as signed (rel < 0). This makes the behavior dependent on an implicit unsigned→signed conversion (implementation-defined when the value is not representable), which can break the “old data behind seq_base is rejected” logic—especially around wrap or large gaps. Consider changing tcp_seq_diff to return an int32_t “relative distance” (the same semantic as the prior (int32_t)(seq - base) approach) and update call sites accordingly, or keep it unsigned but remove signed comparisons and use wrap-aware ordering helpers consistently.
| /* Subtract two TCP sequence numbers: a - b (wraps at 2^32) */ | |
| static inline uint32_t tcp_seq_diff(uint32_t a, uint32_t b) | |
| { | |
| if (a >= b) | |
| return a - b; | |
| return UINT32_MAX - (b - a - 1); | |
| /* Subtract two TCP sequence numbers: a - b (wraps at 2^32) | |
| * and interpret the result as a signed relative distance. */ | |
| static inline int32_t tcp_seq_diff(uint32_t a, uint32_t b) | |
| { | |
| /* Sequence arithmetic is modulo 2^32; cast to int32_t to get | |
| * a signed relative distance in the range [-2^31, 2^31-1]. */ | |
| return (int32_t)(a - b); |
| rel = tcp_seq_diff(seq, q->seq_base); | ||
| if (rel < 0) { |
There was a problem hiding this comment.
tcp_seq_diff() returns uint32_t, but at least one call site treats the result as signed (rel < 0). This makes the behavior dependent on an implicit unsigned→signed conversion (implementation-defined when the value is not representable), which can break the “old data behind seq_base is rejected” logic—especially around wrap or large gaps. Consider changing tcp_seq_diff to return an int32_t “relative distance” (the same semantic as the prior (int32_t)(seq - base) approach) and update call sites accordingly, or keep it unsigned but remove signed comparisons and use wrap-aware ordering helpers consistently.
| /* Reject SYNs that match an already-active connection | ||
| * with the same tuple (local_port, remote_port, remote_ip), that is already | ||
| * in the established state. | ||
| * */ | ||
| for (int k = 0; k < MAX_TCPSOCKETS; k++) { | ||
| struct tsocket *tk = &S->tcpsockets[k]; | ||
| if (tk == t) | ||
| continue; | ||
| if (tk->sock.tcp.state <= TCP_LISTEN || | ||
| tk->sock.tcp.state == TCP_CLOSED) | ||
| continue; | ||
| if (tk->src_port == t->src_port && | ||
| tk->dst_port == ee16(tcp->src_port) && | ||
| tk->remote_ip == ee32(tcp->ip.src)) { | ||
| dup_found = 1; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The duplicate-SYN filter does not include the local IP in the tuple comparison. If the stack can listen on multiple local IPs (or IPADDR_ANY) with the same local port, a SYN to a different local IP could be incorrectly rejected just because an established connection exists with the same remote endpoint and ports. Include the appropriate local identifier(s) in the match (e.g., tk->local_ip/syn_dst, and potentially if_idx if that’s part of the effective demux key in this stack) so only truly identical connections are considered duplicates.
| suite_add_tcase(s, tc_proto); | ||
| tcase_add_test(tc_proto, test_tcp_handshake_and_fin_close_wait); | ||
| suite_add_tcase(s, tc_proto); | ||
|
|
||
|
|
||
| /* Regression tests for fixes-4 branch */ | ||
| tcase_add_test(tc_proto, test_regression_snd_una_initialized_on_syn_rcvd); | ||
| suite_add_tcase(s, tc_proto); | ||
| tcase_add_test(tc_proto, test_regression_duplicate_syn_rejected_on_established); | ||
| suite_add_tcase(s, tc_proto); |
There was a problem hiding this comment.
suite_add_tcase(s, tc_proto) is called multiple times for the same tc_proto object in this block (including the newly added calls). In Check, adding the same TCase* to a suite more than once can lead to duplicated execution or undefined behavior depending on the library/version. The safer pattern is to add tc_proto to the suite once, and only call tcase_add_test() for each additional test.
2 participants
More fixes caught by the fuzzers.